Skip to content

fix: ide-assists, some generate_\* indentations #19789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: ide-assists, some generate_\* indentations #19789

wants to merge 5 commits into from

Conversation

A4-Tacks
Copy link
Contributor

@A4-Tacks A4-Tacks commented May 13, 2025 •
edited
Loading

Fix files:

  • generate_default_from_enum_variant.rs
  • generate_default_from_new.rs
  • generate_deref.rs
  • generate_enum_is_method.rs
  • generate_enum_projection_method.rs
  • generate_getter_or_setter.rs
  • generate_is_empty_from_len.rs
  • generate_from_impl_for_enum.rs

Example:

mod foo {
    enum Foo {
        $0String(String),
    }
}

Old output:

mod foo {
    enum Foo {
        String(String),
    }

impl From<String> for Foo {
    fn from(v: String) -> Self {
        Self::String(v)
    }
}
}

Fixed output:

mod foo {
    enum Foo {
        String(String),
    }

    impl From<String> for Foo {
        fn from(v: String) -> Self {
            Self::String(v)
        }
    }
}

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 13, 2025
davidbarsky
davidbarsky requested changes May 27, 2025
View reviewed changes
@A4-Tacks A4-Tacks requested a review from davidbarsky May 27, 2025 23:20
Veykril
Veykril reviewed May 28, 2025
View reviewed changes
crates/ide-assists/src/utils.rs Outdated
@@ -250,6 +252,36 @@ pub fn add_trait_assoc_items_to_impl(
first_item.unwrap()
}

pub(crate) fn indent_string(mut s: &str, indent_level: IndentLevel) -> Cow<'_, str> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong kind of API here, we already have indentation things that work on the AST. If those don't suffice this will have to wait until we have our own formatter to pretty print nodes with. Our assist output is currently best effort due to the lack of that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, many ide-assists directly generate strings. With this API, I can quickly fix nearly ten ide-assists implements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep this API and continue?

Copy link
Member

@Veykril Veykril Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that it makes us even more reliant on the text based editing, while we want to generate nodes proper and format them with a proper formatter. Until we have a formatter we only try to fix up indentation on a best effort basis.

(on a related note, all the assist PRs you have opened are text based which is also not ideal for the same reason)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(on a related note, all the assist PRs you have opened are text based which is also not ideal for the same reason)

Should I use ted?

Copy link
Member

@Veykril Veykril Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally any new assists should be using the SyntaxEditor infra, see #15710 and #18285 for context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this is that it makes us even more reliant on the text based editing, while we want to generate nodes proper and format them with a proper formatter. Until we have a formatter we only try to fix up indentation on a best effort basis.

Can this PR be merged? It doesn't seem to make things worse

@A4-Tacks A4-Tacks requested a review from Veykril May 28, 2025 21:40
@A4-Tacks A4-Tacks force-pushed the fix-gen-from-impl-for-enum-indent branch from b9e5dff to 1d4eb9e Compare June 1, 2025 11:00
@A4-Tacks A4-Tacks changed the title fix: ide-assists, gen from impl for enum indent fix: ide-assists, some generate_\* indentations Jun 1, 2025
@A4-Tacks
Copy link
Contributor Author

A4-Tacks commented Jun 1, 2025

Fixed more indentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbarsky davidbarsky Awaiting requested review from davidbarsky

@Veykril Veykril Awaiting requested review from Veykril

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@A4-Tacks @davidbarsky @Veykril @rustbot